Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature added: layer_symbol() supports "freetext", and add color, size options #107

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

frankyan
Copy link

Add freetext, color, and size to the layer_symbol() function. Then the layer_symbol() function is much more customizable. It can display values (char) in one column of the data tibble or any other freetext (char) instead of the predefined symbols. Besides, the color and size can be customized.

Here are some examples.

mtcars_tidy %>% 
  mutate(label = sprintf("%.1f", Value),
         color_label = ifelse(Value > 0, "darkred", "white")) %>% 
  heatmap(`Car name`, Property, Value, 
          palette_value = colorRamp2(c(-3, 0, 3), c("blue", "white", "red")))  %>% 
  layer_symbol(Value > 1 | Value < -1,
               symbol = "freetext", 
               freetext = label, 
               color = color_label, 
               size = 10)

image

mtcars_tidy %>% 
  heatmap(`Car name`, Property, Value, 
          palette_value = colorRamp2(c(-3, 0, 3), c("blue", "white", "red")))  %>% 
  layer_symbol(Value > 1,
               symbol = "freetext", 
               freetext = "+", 
               size = 10) %>%
  layer_symbol(Value < -1,
               symbol = "freetext", 
               freetext = "-", 
               size = 15) 

image

mtcars_tidy %>% 
  heatmap(`Car name`, Property, Value, 
          palette_value = colorRamp2(c(-3, 0, 3), c("blue", "white", "red")))  %>% 
  layer_point(Value < -1, color = "yellow")

image

@stemangiola stemangiola self-requested a review January 10, 2023 22:21
Copy link
Owner

@stemangiola stemangiola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @frankyan, for the nice pull request. I think they are an important addition. I provide a few points of feedback on the pull request.

  • Because (i) of modularity, and (ii) the automatic feature update I get automatically from branch names when I create a new release, and (iii) make it more manageable to review, I think this pull request should be separate, through 3 branches, in
  1. addition of size argument for all layer_* functions
  2. addition on color argument for all layer_* functions
  3. addition of text functionality
  • layer_symbol is meant to be hidden, in favour of scope-specific layer_* functions (e.g. layer_text). In this way we modularise and keep arguments clean. So I suggest to implement a layer_text function that would use layer_symbol that you have edited.
layer_text(Value < -1,  text = "-" ) 
  • is there any restriction on the name freetext? If not I would replace freetext to text everywhere in the code

  • for the three functionalities and branches, size, color, and text we need (simple) unit tests

  • for the three functionalities and branches, size, color, and text we need to update the README.Rdm to include one example for each, and possibly the new layer_text function in the list at the beginning of the README

@frankyan
Copy link
Author

I am glad that you are interested in the additional functions. Thank you for your great guides and suggestions.

I will follow your suggestions to modify the code and reorganize the branches.

@stemangiola
Copy link
Owner

Hello @frankyan, any luck with the process? The layer size might have come in handy for me already :)

@frankyan
Copy link
Author

Hello. I just returned back from Chinese New Year vacation. I will try to finish it this week.

@stemangiola
Copy link
Owner

Hello. I just returned back from Chinese New Year vacation. I will try to finish it this week.

I hope you enjoyed. No stress of course.

@stemangiola
Copy link
Owner

Any news by any chance?

@stemangiola
Copy link
Owner

I have started a pull request of my own inspired by yours

#111

@stemangiola
Copy link
Owner

stemangiola commented Mar 23, 2023

FYI, from my pull request, compared with yours, I missed two features

  • size for the non-text other layers
  • colour for text and non-text layers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants